Fix HA scheduler try_number double increment #60330
Conversation
In HA, two scheduler processes can race to schedule the same TaskInstance. Previously DagRun.schedule_tis() updated rows by ti.id alone, so a scheduler could increment try_number and transition state even after another scheduler had already advanced the TI (e.g. to SCHEDULED/QUEUED), resulting in duplicate attempts being queued. This change makes scheduling idempotent under HA races by: - Guarding schedule_tis() DB updates to only apply when the TI is still in schedulable states (derived from SCHEDULEABLE_STATES, handling NULL explicitly). - Using a single CASE (next_try_number) so reschedules (UP_FOR_RESCHEDULE) do not start a new try, and applying this consistently to both normal scheduling and the EmptyOperator fast-path. Adds regression tests covering: - TI already queued by another scheduler. - EmptyOperator fast-path blocked when TI is already QUEUED/RUNNING. - UP_FOR_RESCHEDULE scheduling keeps try_number unchanged. - Only one “scheduler” update succeeds when competing.
| schedulable_state_clause = or_( | ||
| TI.state.is_(None), | ||
| TI.state.in_(non_null_schedulable_states), | ||
| ) |
There was a problem hiding this comment.
We should do some benchmarking/checking of indexes on this query if you haven't already. TI state is almost certainly indexed, but we should check this with some EXPLAIN ANALYZE in a moderatly sized DB if we have one (something to the region of 1-2m TI rows?)
Might also be worth checking if a schedulable_state_clause = TI.state != TaskInstanceState.SCHEDULED is enough and more-performant?
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
LGTM once we've done some perf testing
|
@ephraimbuddy Good job, we are actually testing this fix right now, keep you guys posted if it helped. The weird thing is it seems (we're not sure yet) that we only have this issue with DAG's that have tasks that use the WinRMOperator. |
|
@ephraimbuddy Unfortunately, even after applying this fix, we still run onto the same issue. |
Major problem with the issue is reliability of reproduction path. I have reproduced it once but trying again, I can't. Do you have steps for reliable reproduction? |
|
Now we also experience the same issue with HttpOperator: |
eladkal
added
the
backport-to-v3-1-test
4 participants
In HA, two scheduler processes can race to schedule the same TaskInstance. Previously DagRun.schedule_tis() updated rows by ti.id alone, so a scheduler could increment try_number and transition state even after another scheduler had already advanced the TI (e.g. to SCHEDULED/QUEUED), resulting in duplicate attempts being queued.
This change makes scheduling idempotent under HA races by:
Guarding schedule_tis() DB updates to only apply when the TI is still in schedulable states (derived from SCHEDULEABLE_STATES, handling NULL explicitly).
Using a single CASE (next_try_number) so reschedules (UP_FOR_RESCHEDULE) do not start a new try, and applying this consistently to both normal scheduling and the EmptyOperator fast-path.
Adds regression tests covering:
Closes: #57618
Note: The reproduction of this issue was based on unit tests